Fix update attempt refreshing pill without actually updating#2168
Fix update attempt refreshing pill without actually updating#2168lawrencecchen merged 3 commits intomainfrom
Conversation
The attemptUpdate() subscriber watched for .updateAvailable state to auto-confirm, but showUpdateFound used setStateAfterMinimumCheckDelay which delays the transition by up to 2 seconds. During that window, dismissUpdateInstallation (from a background probe race) could cancel the pending transition, reverting state to idle without ever confirming. The subscriber then tore down on the transient idle, silently abandoning the update. Fix: move auto-confirm to the Sparkle driver level via an autoInstallOnNextUpdate flag. When set, showUpdateFound immediately calls reply(.install) bypassing the delay entirely. The subscriber is kept as a fallback but no longer tears down on transient idle while the flag is active. Closes #2166
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request refines the termination condition in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR fixes a timing race where Key changes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant C as UpdateController
participant D as UpdateDriver
participant S as Sparkle
C->>C: attemptUpdate()
C->>C: stopAttemptUpdateMonitoring()
C->>D: autoInstallOnNextUpdate = true
C->>C: subscribe to viewModel.$state
C->>S: checkForUpdates()
alt Fast path - flag still set when update found
S-->>D: showUpdateFound(appcastItem, reply)
D->>D: autoInstallOnNextUpdate = false
D->>S: reply(.install) bypasses minimum-check delay
S-->>D: showDownloadInitiated / extracting / ready
D-->>C: subscriber sees progress states then idle, tears down
else Race - dismissUpdateInstallation fires before showUpdateFound
S-->>D: dismissUpdateInstallation()
D->>D: autoInstallOnNextUpdate = false
Note over D: state is .checking, dismiss is ignored
S-->>D: showUpdateFound(appcastItem, reply)
D->>D: setStateAfterMinimumCheckDelay(.updateAvailable)
D-->>C: state -> .updateAvailable after delay
C->>C: fallback subscriber calls state.confirm()
else Failure path
S-->>D: showUpdateNotFoundWithError or showUpdaterError
D->>D: autoInstallOnNextUpdate = false
D-->>C: subscriber tears down on terminal state
end
Reviews (1): Last reviewed commit: "Fix update attempt refreshing pill witho..." | Re-trigger Greptile |
| /// without waiting for the minimum-check-display delay. This prevents | ||
| /// the delayed `.updateAvailable` transition from being preempted by | ||
| /// a `dismissUpdateInstallation` call (e.g. from a background probe race). | ||
| var autoInstallOnNextUpdate: Bool = false |
There was a problem hiding this comment.
Thread-safety of
autoInstallOnNextUpdate not formally guaranteed
The existing SPUUserDriver implementation wraps all viewModel.state mutations in runOnMain, which implies Sparkle may invoke driver callbacks from a background thread. autoInstallOnNextUpdate is now read and written in those same callbacks (showUpdateFound, showUpdateNotFoundWithError, showUpdaterError, dismissUpdateInstallation) without any synchronization — but it is also written from UpdateController on the main thread.
If Sparkle ever calls showUpdateFound or dismissUpdateInstallation off the main thread at the same moment UpdateController sets the flag, there is a formal data race on the Bool. In practice this is likely benign on current Apple hardware (word-aligned Bool reads/writes are atomic), but Swift's memory model does not guarantee this.
Consider adding @MainActor to UpdateDriver or at least to the affected methods, or documenting the threading assumption explicitly so future changes don't inadvertently cross threads. Alternatively, since setStateAfterMinimumCheckDelay already dispatches to main via runOnMain, the autoInstallOnNextUpdate read/write in showUpdateFound could be folded into the same runOnMain block used by the rest of the method body.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Update/UpdateDriver.swift`:
- Around line 53-58: When taking the immediate-install branch
(autoInstallOnNextUpdate), ensure you cancel the pending "checking" timeout
established by setStateAfterMinimumCheckDelay(...) before calling
reply(.install); i.e., clear/invalidate the timer or invoke the existing
cancel/clear function for the check timeout so the delayed timeout cannot later
flip state to .notFound while you proceed with reply(.install).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a8e9870-1a6f-4bdd-8a9b-d35b98868f56
📒 Files selected for processing (2)
Sources/Update/UpdateController.swiftSources/Update/UpdateDriver.swift
| if autoInstallOnNextUpdate { | ||
| autoInstallOnNextUpdate = false | ||
| UpdateLogStore.shared.append("auto-installing update (attemptUpdate)") | ||
| reply(.install) | ||
| return | ||
| } |
There was a problem hiding this comment.
Cancel pending check timeout when taking the immediate-install branch.
On Line 53, this path bypasses setStateAfterMinimumCheckDelay(...), so timeout/check-transition bookkeeping can stay armed. If state remains .checking long enough, the timeout path can still force .notFound unexpectedly.
Proposed fix
if autoInstallOnNextUpdate {
autoInstallOnNextUpdate = false
+ pendingCheckTransition?.cancel()
+ pendingCheckTransition = nil
+ checkTimeoutWorkItem?.cancel()
+ checkTimeoutWorkItem = nil
+ lastCheckStart = nil
UpdateLogStore.shared.append("auto-installing update (attemptUpdate)")
reply(.install)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if autoInstallOnNextUpdate { | |
| autoInstallOnNextUpdate = false | |
| UpdateLogStore.shared.append("auto-installing update (attemptUpdate)") | |
| reply(.install) | |
| return | |
| } | |
| if autoInstallOnNextUpdate { | |
| autoInstallOnNextUpdate = false | |
| pendingCheckTransition?.cancel() | |
| pendingCheckTransition = nil | |
| checkTimeoutWorkItem?.cancel() | |
| checkTimeoutWorkItem = nil | |
| lastCheckStart = nil | |
| UpdateLogStore.shared.append("auto-installing update (attemptUpdate)") | |
| reply(.install) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Update/UpdateDriver.swift` around lines 53 - 58, When taking the
immediate-install branch (autoInstallOnNextUpdate), ensure you cancel the
pending "checking" timeout established by setStateAfterMinimumCheckDelay(...)
before calling reply(.install); i.e., clear/invalidate the timer or invoke the
existing cancel/clear function for the check timeout so the delayed timeout
cannot later flip state to .notFound while you proceed with reply(.install).
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Update/UpdateDriver.swift">
<violation number="1" location="Sources/Update/UpdateDriver.swift:54">
P1: Avoid clearing `autoInstallOnNextUpdate` when dismiss is ignored in `.checking`; this can drop the auto-install intent before `showUpdateFound` fires.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
This reverts commit 1cd842d.
The attemptUpdate() subscriber tore down monitoring whenever it saw .idle after observing progress. During check startup (retry loop, background probe race), state can transiently return to .idle before Sparkle's interactive check begins. The subscriber interpreted this as "check completed" and stopped monitoring, so the auto-confirm for .updateAvailable never fired. Fix: add !state.isIdle to the teardown guard so monitoring only stops on terminal failures (.notFound, .error), not transient idle. Closes #2166
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad5e591178
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Only stop on terminal failure states (.notFound, .error). | ||
| // Don't stop on .idle — the check may still be starting up | ||
| // (e.g. retry loop, background probe finishing). | ||
| guard self.didObserveAttemptUpdateProgress, !state.isInstallable, !state.isIdle else { |
There was a problem hiding this comment.
Stop attempt monitor when state settles back to idle
This new guard excludes .idle from terminal states, so attemptInstallCancellable is never cleared when an attempted install path returns to idle (for example after cancel/restart-later). In that case the subscriber keeps running indefinitely and will still auto-confirm() any future .updateAvailable, which can make a later normal update check install automatically without fresh user intent. Please keep the transient-idle protection for startup races, but still tear down monitoring once an attempt has progressed and then comes back to idle.
Useful? React with 👍 / 👎.
…w-ai#2168) * Fix update attempt refreshing pill without actually updating The attemptUpdate() subscriber watched for .updateAvailable state to auto-confirm, but showUpdateFound used setStateAfterMinimumCheckDelay which delays the transition by up to 2 seconds. During that window, dismissUpdateInstallation (from a background probe race) could cancel the pending transition, reverting state to idle without ever confirming. The subscriber then tore down on the transient idle, silently abandoning the update. Fix: move auto-confirm to the Sparkle driver level via an autoInstallOnNextUpdate flag. When set, showUpdateFound immediately calls reply(.install) bypassing the delay entirely. The subscriber is kept as a fallback but no longer tears down on transient idle while the flag is active. Closes manaflow-ai#2166 * Revert "Fix update attempt refreshing pill without actually updating" This reverts commit 8ff45b55ce3c3b10db998c38eae6de2141f79284. * Fix update attempt refreshing pill without actually updating The attemptUpdate() subscriber tore down monitoring whenever it saw .idle after observing progress. During check startup (retry loop, background probe race), state can transiently return to .idle before Sparkle's interactive check begins. The subscriber interpreted this as "check completed" and stopped monitoring, so the auto-confirm for .updateAvailable never fired. Fix: add !state.isIdle to the teardown guard so monitoring only stops on terminal failures (.notFound, .error), not transient idle. Closes manaflow-ai#2166 --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
Summary
!state.isIdleto theattemptUpdate()subscriber teardown guard so monitoring only stops on terminal failures (.notFound,.error), not on transient.idleduring check startupThe subscriber watched
viewModel.$stateto auto-confirm.updateAvailable. But it also tore down on any non-installable state after observing progress, including.idle. During check startup (retry loop waiting forcanCheckForUpdates, background probe finishing), state can transiently return to.idlebefore Sparkle's interactive check begins. The subscriber interpreted this as a completed check and stopped, so the auto-confirm never fired.Testing
Related
Summary by CodeRabbit